Parse Json param with fallback to default way of parsing i.e. string or stringList #181

snigdhasjg · 2024-08-07T17:01:36Z

Description

Processes a collection of AWS Systems Manager (SSM) Parameters Store parameters, converting them into a dictionary of strings.

This method supports both JSON-formatted parameters as per JsonParameterProcessor. And plain string & StringList as per DefaultParameterProcessor.

The resulting dictionary keys are case-insensitive, as the other 2 implemenation.

Motivation and Context

We have multiple types of SSM gets loaded during application startup.
Mainly Json and Text based. Params are dynamic in nature, so no way to telling if its Json or Text.

We are using this code for more than a week now, so why not merge it in the lib itself.

Testing

Tested in our net8.0 application already. It runs on official docker image mcr.microsoft.com/dotnet/aspnet:8.0-jammy
Tested in net6.0 locally
Couldn't test on other 2 version for netstandard2.0;netcoreapp3.1 as using ARM

Unit tests have been added, validated.

No impact on existing area as new implementation of the IParameterProcessor

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new tests passed
All existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

normj · 2024-08-07T17:20:28Z

src/Amazon.Extensions.Configuration.SystemsManager/JsonOrDefaultParameterProcessor.cs

+
+namespace Amazon.Extensions.Configuration.SystemsManager
+{
+    public class JsonOrDefaultParameterProcessor : DefaultParameterProcessor


We need docs here explaining what this processor does. I would suggest instead of using the term Default here I would use String. So it is obvious to users that this supports either Json or falls back to String if the parameter fails to parse as Json.

Yeah I was thinking that, but then I hold myself cause it's both String and StringList

If you think JsonOrStringParameterProcessor would be good enough I can change that.

Updated the name

It is a valid concern that it is also doing StringList but I feel like String is better then Default. If I read the name JsonOrDefault.. and ignore my knowledge of how things are implemented I would see a name that says Json or some unknown format.

Yeah used JsonOrStringParameterProcessor

added docs in places where I felt, open for suggestion

normj · 2024-08-07T17:21:35Z

src/Amazon.Extensions.Configuration.SystemsManager/JsonOrDefaultParameterProcessor.cs

+            {
+                foreach (var kv in JsonConfigurationParser.Parse(parameter.Value))
+                {
+                    var key = !string.IsNullOrEmpty(keyPrefix) ? ConfigurationPath.Combine(keyPrefix, kv.Key) : kv.Key;


Can you refactor this inner loop logic that is a duplicate inner loop logic in JsonParameterProcessor into a common internal utility method.

Yeah I was thinking on the same line.

I have 3 static methods -
ParseJson can be used in both the new class and json processor.
Other 2 ParseString and ParseStringList can also be extracted out and reused.

Let me work on that

Extracted all 3 methods,
refactored both JsonParameterProcessor and DefaultParameterProcessor

Please review

snigdhasjg · 2024-08-07T20:26:30Z

Changes are done from my end. Once reviewed, I'll squash all commits.

peterrsongg

Checked out the branch, ran the tests myself. Looks good

Passed!  - Failed:     0, Passed:    74, Skipped:     0, Total:    74, Duration: 92 ms - Amazon.Extensions.Configuration.SystemsManager.Tests.dll (netcoreapp3.1)
Test run for C:\Dev\Repos\aws-dotnet-extensions-configuration\test\Amazon.Extensions.Configuration.SystemsManager.Tests\bin\Debug\net6.0\Amazon.Extensions.Configuration.SystemsManager.Tests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.10.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

snigdhasjg · 2024-08-27T22:29:49Z

@normj @peterrsongg have't heard back. any update on this?

snigdhasjg · 2024-10-22T08:09:44Z

@normj any update on this, when can this be merged?

normj

Apologize for the delay. The changes look good but can you add a change file for the release? To do that add a file at .autover/changes/<random-guid>.json with the following content. Feel free to tweak the change log message.

{
  "Projects": [
    {
      "Name": "Amazon.Extensions.Configuration.SystemsManager",
      "Type": "Minor",
      "ChangelogMessages": [
        "Add opt-in JsonOrStringParameterProcessor processor supporting parameters being either strings or JSON"
      ]
    }
  ]
}

…or stringList

…ue is being fetched

snigdhasjg · 2025-02-17T20:16:24Z

@normj rebased and added the file .autover/changes/44bf0928-edf9-4888-aebb-196b6364ffa8.json

normj · 2025-02-21T00:11:05Z

PR was released today as part of version 6.3.0. Thanks for the PR!

normj requested changes Aug 7, 2024

View reviewed changes

muhammad-othman requested a review from peterrsongg August 9, 2024 17:30

snigdhasjg requested a review from normj August 12, 2024 18:11

peterrsongg approved these changes Aug 13, 2024

View reviewed changes

normj requested changes Feb 14, 2025

View reviewed changes

snigdhasjg added 8 commits February 18, 2025 01:43

Parse Json param with fallback to default way of parsing i.e. string …

49ad577

…or stringList

refact: extract common method into utility class

d8049f2

fix: json processing should throw JsonException if not json value

276340a

refactor tests and add more tests to cover muitlple scenarios

e8b82d9

add documentation and test for utility methods

47070ac

use GetKey and GetValue to be able to override how parameter key, val…

e69d16f

…ue is being fetched

fix test naming to adhere to existing naming strategy

86f9027

add autover file

10aa967

ashishdhingra requested a review from normj February 20, 2025 18:10

normj approved these changes Feb 20, 2025

View reviewed changes

normj changed the base branch from master to dev February 20, 2025 20:54

normj merged commit 6acd0f4 into aws:dev Feb 20, 2025
1 check failed

Parse Json param with fallback to default way of parsing i.e. string or stringList #181

Parse Json param with fallback to default way of parsing i.e. string or stringList #181

Uh oh!

Conversation

snigdhasjg commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snigdhasjg Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snigdhasjg commented Aug 7, 2024

Uh oh!

peterrsongg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snigdhasjg commented Aug 27, 2024

Uh oh!

snigdhasjg commented Oct 22, 2024

Uh oh!

normj left a comment

Choose a reason for hiding this comment

Uh oh!

snigdhasjg commented Feb 17, 2025

Uh oh!

Uh oh!

normj commented Feb 21, 2025

Uh oh!

Uh oh!

snigdhasjg commented Aug 7, 2024 •

edited

Loading

snigdhasjg Aug 7, 2024 •

edited

Loading

peterrsongg left a comment •

edited

Loading